Skip to content

[native] Fix crash in PrestoTask::updateExecutionInfoLocked#24380

Merged
amitkdutta merged 1 commit into
prestodb:masterfrom
arhimondr:fix_segenv_task_info
Jan 16, 2025
Merged

[native] Fix crash in PrestoTask::updateExecutionInfoLocked#24380
amitkdutta merged 1 commit into
prestodb:masterfrom
arhimondr:fix_segenv_task_info

Conversation

@arhimondr
Copy link
Copy Markdown
Member

== NO RELEASE NOTE ==

@arhimondr arhimondr requested a review from a team as a code owner January 16, 2025 20:03
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Jan 16, 2025
spershin
spershin previously approved these changes Jan 16, 2025
Copy link
Copy Markdown
Contributor

@spershin spershin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for quickly fixing the issue!

Comment thread presto-native-execution/presto_cpp/main/PrestoTask.cpp
@arhimondr
Copy link
Copy Markdown
Member Author

@spershin Thank you for a prompt review. Updated.

@amitkdutta
Copy link
Copy Markdown
Contributor

@arhimondr Thanks for the quick fix. Wondering why we didn't see the problem in OSS change. Do we need to add any e2e tests that we have in Meta internal system.

@arhimondr
Copy link
Copy Markdown
Member Author

arhimondr commented Jan 16, 2025

@amitkdutta From what I can see this can only happen when query:

  1. uses grouped execution (for non grouped execution drivers are created eagerly: https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L683, for grouped execution lazily when splits are received: https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L1284, https://github.com/facebookincubator/velox/blob/main/velox/exec/Task.cpp#L1390)
  2. Initial TaskUpdateRequest has no splits

I guess 1 is why I didn't catch it when shadowing thousands of queries. I assume I just didn't get to to run a lot of grouped execution queries.

And the 2 is why tests are not deterministically failing.

@amitkdutta amitkdutta merged commit ae68ae4 into prestodb:master Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants